Pull Request Review: Clean up Nix configuration and remove unused paths

Summary

This is an excellent cleanup PR that removes significant technical debt from the repository. The changes are well-organized and properly documented in the PR description.

✅ Strengths

1. Massive Reduction in Complexity (10,478 deletions vs 26 additions)

   • Removes ~30 orphaned daemon service files
   • Eliminates 2 deprecated system configurations (singapore, austria)
   • Cleans up unused flake inputs
   • This will significantly improve maintainability

2. Proper Migration to Modern Architecture

   • Individual arr services (sonarr, radarr, lidarr) replaced by containerized arr.nix
   • This is the correct architectural direction for self-hosted services

3. Documentation Updates

   • CLAUDE.md properly updated to reflect current repository state
   • Machine table correctly shows only active systems
   • Repository structure documentation cleaned up

4. Re-enabling Critical Server Features (systems/nixos/china/configuration.nix:12-13)

   • system.autoUpgrade and jpyke3.scheduleReboot restored
   • These were likely disabled during testing and correctly re-enabled

🔍 Code Quality & Best Practices

Excellent:

• No half-measures: removes entire unused systems rather than commenting them out
• Clean removal of unused flake inputs (llama-cpp, base16, spicetify-nix, nixos-apple-silicon)
• Consistent with Nix best practices (no orphaned files left behind)

⚠️ Considerations & Recommendations

1. Binary Cache CI Pipeline (Medium Priority)

The PR removes the jacob-austria and jacob-singapore systems, but the CI pipeline in .github/workflows/build.yaml may still reference these. Please verify:

# Check if CI needs updating
grep -r "singapore\|austria" .github/workflows/

If found: Remove these from the build matrix to avoid CI failures.

2. Verify Deleted Services Are Actually Unused (Low Priority)

While the PR description states these are "orphaned," double-check that no active machine imports these services:

# Check for any remaining imports of deleted services
rg -l "(sonarr|radarr|lidarr|transmission|deluge|nextcloud|immich|gitea)\.nix" systems/

Expected result: No matches (if matches found, they should be removed or migrated to arr.nix).

3. Austria Firmware Files (Note)

The PR removes binary firmware files (systems/nixos/austria/firmware/*.tar.gz, kernelcache.release.mac13j). These are large binary files that should ideally never have been committed to Git (they bloat repository history permanently).

Recommendation: Consider running git filter-repo or BFG Repo-Cleaner to purge these from Git history to reduce clone size (this is optional and can be done later).

4. Darwin Configuration Leftover? (Very Low Priority)

systems/darwin/configuration.nix has 3 deleted lines but file still exists. Verify this is intentional and the file is still actively used by jacob-germany.

🔒 Security Review

No security concerns identified:

• No credential exposure
• No new services introduced
• Removal of services reduces attack surface (security improvement)
• Secrets management (sops-nix) unchanged

🚀 Performance

Significant improvements:

• Reduced flake evaluation time (fewer inputs to fetch)
• Faster nix flake check (fewer configurations to build)
• Smaller repository clone size (large firmware files removed)

🧪 Testing Recommendations

Before merging, please verify:

1. Build all active systems locally:

   nix build .#nixosConfigurations.jacob-norway.config.system.build.toplevel
   nix build .#nixosConfigurations.jacob-china.config.system.build.toplevel
   nix build .#nixosConfigurations.jacob-japan.config.system.build.toplevel
   nix build .#darwinConfigurations.jacob-germany.system

2. Check for dead references:

   # Run deadnix (you have CI for this)
   nix run nixpkgs#deadnix -- -e

3. Verify china auto-upgrade works:
   After deploying, check that auto-upgrade is functional:

   # On jacob-china
   systemctl status nixos-upgrade.timer
   journalctl -u nixos-upgrade.service

📊 Overall Assessment

✅ Recommendation: APPROVE with minor verification

This PR is ready to merge after verifying:

1. All active systems build successfully
2. CI pipeline doesn't reference removed systems
3. No remaining imports of deleted service files

Great work on this cleanup! Removing 10k+ lines of unused code is a major win for repository maintainability.

📝 Commit Message Suggestion

If you want to follow conventional commits style:

chore: remove deprecated systems and unused service configurations

- Remove singapore (gaming desktop) and austria (Asahi Linux) systems
- Remove ~30 orphaned daemon service files superseded by containerized arr.nix
- Clean up unused flake inputs (llama-cpp, base16, spicetify-nix, etc.)
- Re-enable china server auto-upgrade and scheduled reboot
- Update CLAUDE.md to reflect current repository state

BREAKING CHANGE: singapore and austria system configurations removed

Review completed. Feel free to reach out if you have questions about any of these recommendations!

Code Review: Clean up Nix configuration and remove unused paths

Summary

This is an excellent cleanup PR that removes ~10,500 lines of dead code while improving maintainability. The changes align well with the repository's current infrastructure direction (containerized services via arr.nix, self-hosted binary cache, etc.).

✅ Strengths

1. Comprehensive Dead Code Removal

• ✅ 30+ orphaned daemon services removed (individual arr services superseded by containerized arr.nix)
• ✅ Deprecated systems (singapore, austria) fully removed with no dangling references
• ✅ Unused flake inputs cleaned up (llama-cpp, spicetify-nix, nixos-apple-silicon, etc.)
• ✅ Orphaned CLI/desktop programs removed (ollama.nix, spicetify.nix never imported)

2. Infrastructure Improvements

• ✅ Re-enabled critical server functionality on jacob-china:
  • system.autoUpgrade (daily GitHub pulls at 03:00) - systems/nixos/china/configuration.nix:88-94
  • jpyke3.scheduleReboot (daily at 03:00) - systems/nixos/china/configuration.nix:23-26
• ✅ These changes ensure the home server stays up-to-date automatically

3. Documentation Accuracy

• ✅ CLAUDE.md updated to reflect current state
• ✅ Removed references to deprecated systems
• ✅ Clarified containerized architecture (arr.nix for media stack)

4. Clean Git History

• ✅ Removed commented-out system definitions from flake.nix
• ✅ No half-deleted code or "deprecated but kept" anti-patterns

⚠️ Potential Issues

1. Orphaned User Group References (systems/nixos/china/configuration.nix:56-86)

These user group definitions reference services that were removed:

documents group: firefly, nextcloud, immich (all removed)
git group: gitea (removed)
media-server group: sonarr, radarr, deluge, lidarr, transmission, bazarr, immich (removed or containerized)

Impact: These users no longer exist on the system, which could cause issues during system rebuilds.

Recommendation: Remove references to deleted services. Keep only:

• jacobpyke (always present)
• vaultwarden (still enabled)
• komga (still enabled)
• Container users from arr.nix if they exist

2. Missing Verification of Container User Mapping

Since individual arr services were replaced with containerized arr.nix, verify:

• Do containers create system users?
• Are file permissions correct for NFS mounts?
• Does media-server group need container users?

Recommendation: Check programs/daemon/arr.nix to confirm container user/group setup matches the old configuration.

🔒 Security Considerations

✅ Good Practices Maintained

• Secrets management via SOPS unchanged
• Firewall configuration remains restrictive
• Tailscale trusted interface preserved
• Auto-upgrade pulls from GitHub

⚠️ Minor Concern: Auto-Upgrade + Auto-Reboot

The server now auto-upgrades and auto-reboots daily at 3 AM from the main branch.

Risk: If a bad commit is pushed to main, the server will auto-upgrade and reboot into a broken state.

Mitigations (Optional):

1. Add rebootWindow to avoid rebooting during critical times
2. Consider adding a smoke test before reboot
3. Use GitHub branch protection to require CI passing before merge

🚀 Performance Considerations

✅ Positive Impact

• Faster evaluations: Removing unused flake inputs reduces lock file size and evaluation time
• Cleaner build cache: Fewer derivations means better cache hit rates
• Reduced attack surface: Fewer services = less to maintain/secure

ℹ️ No Performance Regressions Expected

• Containerized arr stack should have similar performance to native services
• Binary cache infrastructure unchanged

🧪 Test Coverage

⚠️ Testing Recommendations

Since this PR removes significant infrastructure, recommend testing on jacob-china before auto-upgrade:

1. Build test: nix build for jacob-china config
2. Verify no broken references: nix flake check
3. Check for unused user/group errors: nixos-rebuild dry-build
4. Manual deployment first: Deploy manually and verify media stack, Attic, Vaultwarden, and NFS mounts
5. Only after successful manual test: Enable auto-upgrade

📋 Code Quality

✅ Excellent Practices

• Consistent file naming conventions maintained
• Proper Nix formatting
• Clear commit message explaining all removals
• CLAUDE.md updated (critical for future maintainers)

ℹ️ Minor Suggestion

Consider adding comments in china/configuration.nix explaining why certain group members are kept.

🎯 Final Recommendation

APPROVE ✅ (with minor cleanup suggested)

This PR successfully removes dead code and improves maintainability. The only actionable issue is the orphaned user group references.

Action Items:

1. Required: Clean up systems/nixos/china/configuration.nix user groups (lines 56-86) to remove references to deleted services
2. Optional: Test manual deployment on jacob-china before relying on auto-upgrade
3. Optional: Consider adding reboot window constraints to system.autoUpgrade

Merge Safety:

• ✅ No security vulnerabilities introduced
• ✅ No breaking changes to other systems (norway, japan, jacob-germany)
• ⚠️ jacob-china needs verification (orphaned group members)

Great work on this cleanup! 🎉 Removing 10k+ lines of dead code is a significant maintenance win.

Code Review: Clean up Nix configuration and remove unused paths

Summary

This is a well-executed cleanup PR that removes a substantial amount of deprecated code (~10,400 lines deleted vs 38 added). The changes align with the repository's current state and properly document the active infrastructure.

✅ Strengths

1. Thorough Cleanup

• Removed two complete deprecated systems (singapore, austria) including all associated hardware configs and Apple Silicon support
• Removed ~30 orphaned daemon service files that were superseded by containerized solutions
• Cleaned up unused CLI/desktop programs
• Removed 4 unused flake inputs (llama-cpp, base16, spicetify-nix, nixos-apple-silicon)

2. Good Documentation

• CLAUDE.md updated to accurately reflect current machine inventory
• Repository structure documentation matches actual codebase
• Clear PR description explaining what was removed and why

3. Correct Syncthing Migration

• Properly renamed device references from singapore → norway
• Removed stale netherlands device references
• All 9 Syncthing folder configs updated consistently

4. Proper lib.mkForce Usage

• Third commit correctly uses lib.mkForce for allowReboot override on china
• This resolves the conflict with shared configuration.nix properly

⚠️ Issues & Concerns

1. Critical: Orphaned User Group Members (systems/nixos/china/configuration.nix:57-87)

The cleanup removed services but left their users in group memberships:

users.groups.documents = {
  members = [
    "jacobpyke"
    "firefly"       # ← firefly-iii.nix deleted
    "vaultwarden"
    "nextcloud"     # ← nextcloud.nix deleted
    "immich"        # ← immich.nix deleted
  ];
};

users.groups.git = {
  members = [
    "jacobpyke"
    "gitea"         # ← gitea.nix deleted
  ];
};

users.groups.media-server = {
  members = [
    "sonarr"        # ← now containerized (PUID 1000)
    "radarr"        # ← now containerized (PUID 1000)
    "deluge"        # ← arr.nix uses deluge service, this is OK
    "lidarr"        # ← lidarr.nix deleted
    "jellyfin"      # ← jellyfin.nix deleted
    "transmission"  # ← transmission.nix deleted
    "jacobpyke"
    "bazarr"        # ← arr.nix uses bazarr service, this is OK
    "immich"        # ← immich.nix deleted
    "komga"         # ← komga.nix exists, this is OK
  ];
};

Impact: When NixOS tries to create these groups, it may fail if the referenced users don't exist (created by deleted services). This could cause rebuild failures.

Recommendation: Remove orphaned users from group memberships:

• documents group: Remove firefly, nextcloud, immich
• git group: Remove gitea (or remove entire group if unused)
• media-server group: Remove sonarr, radarr, lidarr, jellyfin, transmission, immich

2. Consistency: Auto-upgrade on china

The PR re-enables auto-upgrade and reboot on china, which is good for a server. However, verify:

• GitHub Actions workflow still builds china successfully (appears so from build.yaml)
• The 03:00 timing doesn't conflict with any critical services
• Consider adding a pre-upgrade health check or notification

3. Minor: CLAUDE.md Accuracy (CLAUDE.md:426)

The build pipeline description mentions building jacob-japan, but the actual workflow builds jacob-norway, jacob-china, and jacob-japan. This is correct, just noting that japan uses the unstable channel which may have different caching behavior.

🔍 Security & Best Practices

✅ Good Practices Observed

• Proper use of SOPS for secrets management maintained
• Containerized services use non-root PUID/PGID (1000)
• Firewall rules remain restrictive with Tailscale trusted interface
• No hardcoded credentials in removed files

⚠️ Minor Concern: Permissive Media Permissions

In arr.nix, systemd tmpfiles rules use 0775 permissions and UID 1000. While this is standard for media servers, ensure:

• UID 1000 is actually the jacobpyke user on china
• The media-server group has the correct GID

🧪 Testing Recommendations

Before merging, verify on a test system or china directly:

# 1. Check for syntax errors
nix flake check

# 2. Dry-run build for china
nixos-rebuild dry-build --flake .#jacob-china

# 3. Verify no undefined users in groups
# (This will show errors if firefly, gitea, etc. don't exist)
nixos-rebuild build --flake .#jacob-china --show-trace

📊 Performance & Maintenance

✅ Positive Impact

• Reduced evaluation time: 10K fewer lines = faster flake evaluation
• Clearer mental model: Only active systems remain
• Easier maintenance: No confusion about deprecated singapore/austria configs

✅ Binary Cache Impact

The cleanup doesn't affect the Attic/Cachix infrastructure since it only removes code, not builds.

📝 Recommendations Summary

Must Fix Before Merge:

1. Remove orphaned users from group memberships in systems/nixos/china/configuration.nix

Should Consider:

2. Verify china rebuild succeeds with updated group memberships
3. Test auto-upgrade behavior on china (or monitor first auto-upgrade closely)
4. Consider adding a comment explaining why certain users remain in groups (e.g., deluge, bazarr from arr.nix)

Nice to Have:

5. Add a comment in arr.nix explaining PUID 1000 = jacobpyke for future reference

Final Verdict

Overall Assessment: ⭐⭐⭐⭐ (4/5)

This is a high-quality cleanup that significantly improves repository maintainability. The main blocker is the orphaned user references in group memberships, which could cause build failures. Once that's resolved, this is ready to merge.

Suggested Action:

1. Fix the group membership issue
2. Test rebuild on china
3. Merge and monitor the first auto-upgrade

Great work on this cleanup! 🎉

PR Review: Cleanup Nix Configuration

Summary

This is an excellent cleanup PR that removes significant technical debt from the repository. The changes are well-documented and align with the current infrastructure state.

✅ Strengths

1. Comprehensive Cleanup

• Successfully removed 2 deprecated systems (singapore, austria) with ~80 files total
• Removed ~30 orphaned daemon services that were superseded by containerized solutions
• Cleaned up unused CLI/desktop programs
• Removed 4 unused flake inputs, reducing dependency complexity

2. Documentation Updates

• CLAUDE.md accurately reflects the current state of the repository
• Clear documentation of the arr.nix containerized approach
• Updated machine naming table and system-specific notes

3. Re-enabled Critical Features

• system.autoUpgrade on china (daily at 03:00) - ensures server stays updated
• jpyke3.scheduleReboot on china (daily at 03:00) - good for applying kernel updates
• Both use GitHub as source, maintaining GitOps best practices

4. Code Quality

• No syntax errors detected
• Consistent with Nix best practices
• Proper use of lib.mkForce for override in systems/nixos/china/configuration.nix:91

⚠️ Issues & Concerns

CRITICAL: Orphaned User Groups

Location: systems/nixos/china/configuration.nix:57-87

The PR correctly removed individual service files (sonarr.nix, radarr.nix, etc.), but the user group definitions still reference non-existent service users:

users.groups.documents = {
  members = [
    "jacobpyke"
    "firefly"      # ❌ firefly-iii.nix removed
    "vaultwarden"  # ✅ Still exists
    "nextcloud"    # ❌ nextcloud.nix removed
    "immich"       # ❌ immich.nix removed
  ];
};

users.groups.git = {
  members = [
    "jacobpyke"
    "gitea"        # ❌ gitea.nix removed
  ];
};

users.groups.media-server = {
  members = [
    "sonarr"       # ❌ Now containerized (uid 1000)
    "radarr"       # ❌ Now containerized (uid 1000)
    "deluge"       # ✅ Still NixOS service
    "lidarr"       # ⚠️ Not found (was lidarr.nix removed?)
    "jellyfin"     # ⚠️ Not found (was jellyfin.nix removed?)
    "transmission" # ❌ transmission.nix removed
    "jacobpyke"    # ✅ Still exists
    "bazarr"       # ✅ Still NixOS service (arr.nix:32)
    "immich"       # ❌ immich.nix removed
    "komga"        # ⚠️ Need to verify if komga.nix still exists
  ];
};

Impact:

• These orphaned group memberships won't cause build failures
• However, they create confusion and may cause permission issues if services are re-added
• The containerized services now run as uid 1000 (arr.nix:109), not as named users

Recommendation:
Clean up the user groups to only include:

users.groups.documents = {
  members = ["jacobpyke" "vaultwarden"];
};

users.groups.media-server = {
  members = ["jacobpyke" "deluge" "bazarr" "komga"];
};

# Consider removing git group entirely if gitea is gone

Minor: Syncthing Device ID Format

Location: programs/daemon/syncthing/folders/*.nix

The changes from "singapore" to "china" look correct, but ensure the device IDs in syncthing.nix:2 match the actual device IDs in use. This is just a reminder to verify the device IDs are correct after deployment.

📋 Testing Recommendations

1. Build Test (should pass):

   nix build .#nixosConfigurations.jacob-china.config.system.build.toplevel
   nix build .#nixosConfigurations.jacob-norway.config.system.build.toplevel
   nix build .#nixosConfigurations.jacob-japan.config.system.build.toplevel

2. Deployment Test (for china):

   • Verify arr.nix containers start successfully
   • Check file permissions on /media mounts (should still work with uid 1000)
   • Verify auto-upgrade executes at 03:00
   • Confirm scheduled reboot works at 03:00

3. Post-deployment Verification:

   # Check for undefined users in group definitions
   systemctl status podman-sonarr-tv
   ls -la /media/TV\ Shows/
   journalctl -u nixos-upgrade.service

🔒 Security Considerations

✅ No security issues identified:

• No secrets exposed in removed files
• Proper use of sops-nix secrets
• Firewall configuration remains intact
• Nginx reverse proxy properly configured with SSL

🚀 Performance Considerations

✅ Positive impact:

• Reduced flake evaluation time (fewer inputs)
• Smaller closure size (unused packages removed)
• Better organization with containerized services

✨ Best Practices Alignment

✅ Follows repository conventions:

• Geographic machine naming maintained
• Proper separation of concerns (programs/ vs systems/)
• Consistent import patterns
• Documentation updates match code changes

📝 Recommendations

High Priority

1. Fix orphaned user groups (see Critical section above)

Medium Priority

2. Consider adding a comment in china/configuration.nix explaining the historical group memberships are deprecated
3. Verify all referenced services in nginx reverse proxy still exist (some may point to removed services)

Low Priority

4. Consider adding a "Migration Notes" section to CLAUDE.md documenting:
   • When singapore/austria were deprecated
   • Why containerized approach was chosen for arr services
   • How to add new arr service instances

🎯 Final Verdict

Status: Approve with minor fixes recommended

This PR successfully achieves its goal of cleaning up the repository. The only blocking issue is the orphaned user group memberships in china/configuration.nix, which should be cleaned up to avoid confusion.

The PR demonstrates good engineering practices:

• Clear commit message
• Comprehensive scope
• Documentation updates
• No breaking changes to active systems

Great work on this cleanup! 🎉

Action Items:

• Clean up orphaned user group memberships in systems/nixos/china/configuration.nix
• Test build all systems before merge
• Monitor china server after deployment (auto-upgrade, reboot schedule)